Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add unregister nano contract saga effect [10] #457

Merged
merged 4 commits into from
May 29, 2024
Merged

feat: add unregister nano contract saga effect [10] #457

merged 4 commits into from
May 29, 2024

Conversation

alexruzenhack
Copy link
Contributor

@alexruzenhack alexruzenhack commented Apr 1, 2024 •
edited
Loading

Motivation

This is a PR on a sequence to make the review easier.

Acceptance Criteria

  • It should implement the unregister saga effect for nano contract
  • It should add unregisterNanoContract to HybridStore
  • It should clean registered nano contracts on cleanTokens
  • It should update payload of nanoContractAddressChangeRequest action
  • It should implement reducer onNanoContractAddressChangeRequest

Note

It depends on:

Security Checklist

  • Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.

@alexruzenhack alexruzenhack self-assigned this Apr 1, 2024
@alexruzenhack alexruzenhack mentioned this pull request Apr 1, 2024
Merged
1 task
@alexruzenhack alexruzenhack changed the base branch from feat/nano-contract-list-component to feat/nc-add-components April 5, 2024 17:39
@alexruzenhack alexruzenhack changed the title feat: add unregister nano contract saga effect feat: add unregister nano contract saga effect [10] Apr 8, 2024
@alexruzenhack alexruzenhack force-pushed the feat/nc-add-components branch 3 times, most recently from 2c4494b to cbd98be Compare May 24, 2024 13:49
Base automatically changed from feat/nc-add-components to master May 28, 2024 15:59
@alexruzenhack alexruzenhack force-pushed the feat/nc-unregister-saga branch 4 times, most recently from e5fdf24 to a56f2fc Compare May 28, 2024 20:18
pedroferreira1
pedroferreira1 reviewed May 29, 2024
View reviewed changes
src/sagas/nanoContract.js Outdated Show resolved Hide resolved
src/store.js
async unregisterNanoContract(ncId) {
await super.unregisterNanoContract(ncId);
const contracts = STORE.getItem(REGISTERED_NANO_CONTRACTS_KEY) || {};
delete contracts[ncId];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you need to set the new object to STORE?

Copy link
Contributor Author

@alexruzenhack alexruzenhack May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't. There aren't a new object to set, contracts is a reference to the object, and we only need remove an entry from this object to set it right. Unless we want to work with immutable objects, which is not the case here. The unregisterToken method sets the registerTokens object after delete, but this operation is redundant.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that the STORE allows us to mutate its stored object directly, but I don't like the idea of potentially multiple places mutating the object directly, I think we should indeed use immutable objects

My suggestion is that we avoid mutating the object in this PR, use STORE.setItem to store it even though it's redundant and open a KTLO to return immutable objects in the Storage

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree we should do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestion is that we avoid mutating the object in this PR, use STORE.setItem to store it even though it's redundant and open a KTLO to return immutable objects in the Storage

Ok. I will open a PR. However, to achieve immutability we need more than just use the spread operator.

Copy link
Contributor

@andreabadesso andreabadesso May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand, we can discuss this in the KTLO, but at the very least we should return a new instance of the object, to prevent the stored object from being mutated by the api consumer

src/actions.js Outdated Show resolved Hide resolved
src/actions.js Show resolved Hide resolved
src/reducers/reducer.js Outdated Show resolved Hide resolved
@alexruzenhack
feat(nc): implement unregister nano contract saga effect
083f44e 
fix: docstring

fix: registerNanoContract test

fix: historyNanoContract test

feat: remove registered nano contracts on cleanTokens

feat: add unregisterNanoContract to HybridStore

review: apply suggestions and do some tweaks
andreabadesso
andreabadesso requested changes May 29, 2024
View reviewed changes
src/store.js
async unregisterNanoContract(ncId) {
await super.unregisterNanoContract(ncId);
const contracts = STORE.getItem(REGISTERED_NANO_CONTRACTS_KEY) || {};
delete contracts[ncId];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that the STORE allows us to mutate its stored object directly, but I don't like the idea of potentially multiple places mutating the object directly, I think we should indeed use immutable objects

My suggestion is that we avoid mutating the object in this PR, use STORE.setItem to store it even though it's redundant and open a KTLO to return immutable objects in the Storage

src/sagas/nanoContract.js Outdated Show resolved Hide resolved
src/sagas/nanoContract.js Outdated

const wallet = yield select((state) => state.wallet);
if (!wallet.isReady()) {
log.debug('Fail unregistering Nano Contract because wallet is not ready yet.');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use log.error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we shouldn't because there was no error, it is just a condition we don't tolerate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an exception, we even yield a fatal error to sentry

andreabadesso
andreabadesso approved these changes May 29, 2024
View reviewed changes
src/sagas/nanoContract.js Outdated

const wallet = yield select((state) => state.wallet);
if (!wallet.isReady()) {
log.debug('Fail unregistering Nano Contract because wallet is not ready yet.');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an exception, we even yield a fatal error to sentry

@alexruzenhack alexruzenhack mentioned this pull request May 29, 2024
Open
@alexruzenhack alexruzenhack merged commit f884a85 into master May 29, 2024
2 checks passed
@alexruzenhack alexruzenhack deleted the feat/nc-unregister-saga branch May 29, 2024 23:29
andreabadesso pushed a commit that referenced this pull request Jun 24, 2024
@alexruzenhack @andreabadesso
feat: add unregister nano contract saga effect [10] (#457)
ed252db 
* feat(nc): implement unregister nano contract saga effect
* feat: remove registered nano contracts on cleanTokens
* feat: add unregisterNanoContract to HybridStore
@alexruzenhack alexruzenhack mentioned this pull request Oct 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pedroferreira1 pedroferreira1 pedroferreira1 approved these changes

@andreabadesso andreabadesso andreabadesso approved these changes

Assignees

@alexruzenhack alexruzenhack

Labels
None yet
Projects
Hathor Network
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alexruzenhack @pedroferreira1 @andreabadesso